feat(cli): add implementation plan and project scaffolding for Terminal49 CLI#177
feat(cli): add implementation plan and project scaffolding for Terminal49 CLI#177dodeja wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aa09382aff
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "main": "dist/index.js", | ||
| "types": "dist/index.d.ts", |
There was a problem hiding this comment.
Point main/types to compiled path
With tsconfig.json using rootDir: "." and include: ["src/**/*", "bin/**/*"], the compiled entry for src/index.ts will land at dist/src/index.js (and dist/src/index.d.ts). The current main/types fields point to dist/index.*, which won’t exist after tsc runs. Any consumer doing import/require of the package entry will fail at runtime or during type resolution. Adjust these fields to the emitted path (or change rootDir/outDir layout) to avoid broken package entrypoints.
Useful? React with 👍 / 👎.
…al49 CLI Comprehensive plan for building a CLI on top of @terminal49/sdk targeting LLM agents, chat interfaces, and human developers. Includes: - PLAN.md: Full architecture, command surface, output modes, LLM agent design patterns, error handling, and 4-phase implementation roadmap - Test plan: ~141 tests across unit/integration/e2e with 90%+ coverage targets enforced via vitest thresholds - Project scaffold: package.json, tsconfig, biome, vitest config, and directory structure with annotated placeholder modules https://claude.ai/code/session_01361NQzDnx8FQSPizmZmjrW
aa09382 to
deae817
Compare
| const program = createProgram(); | ||
| program.parse(process.argv); |
There was a problem hiding this comment.
program.parse will not await async command handlers
All CLI commands will make HTTP requests via the SDK (async operations). Commander's program.parse() does not await async .action() handlers — they run as fire-and-forget Promises, meaning errors are silently swallowed and process.exit may be called before the async work completes.
This should use parseAsync at the entry point:
| const program = createProgram(); | |
| program.parse(process.argv); | |
| import { createProgram } from '../src/index.js'; | |
| const program = createProgram(); | |
| await program.parseAsync(process.argv); |
Because of the top-level await, bin/t49.ts must also be compiled with a target/module that supports it (already satisfied by "target": "ES2022" + "module": "NodeNext").
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdks/typescript-sdk-cli/bin/t49.ts
Line: 12-13
Comment:
**`program.parse` will not await async command handlers**
All CLI commands will make HTTP requests via the SDK (async operations). Commander's `program.parse()` does **not** await async `.action()` handlers — they run as fire-and-forget Promises, meaning errors are silently swallowed and `process.exit` may be called before the async work completes.
This should use `parseAsync` at the entry point:
```suggestion
import { createProgram } from '../src/index.js';
const program = createProgram();
await program.parseAsync(process.argv);
```
Because of the top-level `await`, `bin/t49.ts` must also be compiled with a target/module that supports it (already satisfied by `"target": "ES2022"` + `"module": "NodeNext"`).
How can I resolve this? If you propose a fix, please make it concise.| // Global flags | ||
| .option('--json', 'Force JSON output') | ||
| .option('--table', 'Force table output') | ||
| .option('--raw', 'Output raw JSON:API document') |
There was a problem hiding this comment.
Hardcoded version will drift from package.json
.version('0.1.0') duplicates the version from package.json. As the package is bumped these will diverge silently. Since resolveJsonModule: true is already set in tsconfig.json, the version can be imported directly:
import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);
const { version } = require('../package.json');
Then replace the hardcoded string:
| .option('--raw', 'Output raw JSON:API document') | |
| .version(version) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdks/typescript-sdk-cli/src/index.ts
Line: 22
Comment:
**Hardcoded version will drift from `package.json`**
`.version('0.1.0')` duplicates the version from `package.json`. As the package is bumped these will diverge silently. Since `resolveJsonModule: true` is already set in `tsconfig.json`, the version can be imported directly:
```
import { createRequire } from 'node:module';
const require = createRequire(import.meta.url);
const { version } = require('../package.json');
```
Then replace the hardcoded string:
```suggestion
.version(version)
```
How can I resolve this? If you propose a fix, please make it concise.| .option('--fields <fields>', 'Comma-separated field projection') | ||
| .option('--token <token>', 'API token (overrides env/config)') | ||
| .option('--base-url <url>', 'API base URL override') | ||
| .option('--format <format>', 'Response format: raw | mapped | both', 'mapped') | ||
| .option('-q, --quiet', 'Suppress non-data output') | ||
| .option('-v, --verbose', 'Verbose diagnostics to stderr') |
There was a problem hiding this comment.
--raw flag duplicates --format raw
Line 24 adds a standalone --raw flag ("Output raw JSON:API document"), while line 29 adds --format <format> which already accepts raw as a valid value. These two flags represent the same output mode. Having both creates ambiguous precedence (what wins when --raw and --format mapped are both passed?) and will confuse both human users and LLM agents discovering the CLI via t49 commands --json.
Consider removing --raw and making --format raw the canonical way to request the raw JSON:API document.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdks/typescript-sdk-cli/src/index.ts
Line: 24-29
Comment:
**`--raw` flag duplicates `--format raw`**
Line 24 adds a standalone `--raw` flag ("Output raw JSON:API document"), while line 29 adds `--format <format>` which already accepts `raw` as a valid value. These two flags represent the same output mode. Having both creates ambiguous precedence (what wins when `--raw` and `--format mapped` are both passed?) and will confuse both human users and LLM agents discovering the CLI via `t49 commands --json`.
Consider removing `--raw` and making `--format raw` the canonical way to request the raw JSON:API document.
How can I resolve this? If you propose a fix, please make it concise.| .option('--table', 'Force table output') | ||
| .option('--raw', 'Output raw JSON:API document') | ||
| .option('--compact', 'Minified JSON (reduces LLM token usage)') |
There was a problem hiding this comment.
--json and --table are mutually exclusive but not enforced
Both --json and --table can be passed simultaneously. Commander does not auto-detect conflicting flags, so the formatter will receive both as true with no clear winner. Even as a scaffold, adding a note or using a Commander .addOption with .conflicts() will prevent future confusion:
.addOption(new Option('--json', 'Force JSON output').conflicts('table'))
.addOption(new Option('--table', 'Force table output').conflicts('json'))Prompt To Fix With AI
This is a comment left during a code review.
Path: sdks/typescript-sdk-cli/src/index.ts
Line: 21-23
Comment:
**`--json` and `--table` are mutually exclusive but not enforced**
Both `--json` and `--table` can be passed simultaneously. Commander does not auto-detect conflicting flags, so the formatter will receive both as `true` with no clear winner. Even as a scaffold, adding a note or using a Commander `.addOption` with `.conflicts()` will prevent future confusion:
```ts
.addOption(new Option('--json', 'Force JSON output').conflicts('table'))
.addOption(new Option('--table', 'Force table output').conflicts('json'))
```
How can I resolve this? If you propose a fix, please make it concise.| "devDependencies": { | ||
| "typescript": "^5.6.3", |
There was a problem hiding this comment.
vitest version pinned to ^4.0.13 — verify this is intentional
Vitest ^4.x is a very recent major release. Confirm this version is available and stable in your environment; the ^ range will pick up any 4.x patch/minor automatically, which may be fine, but it is worth validating the major version bump from the more widely-used ^2.x series is deliberate and that the API used in vitest.config.ts (configDefaults, defineConfig) is still compatible.
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdks/typescript-sdk-cli/package.json
Line: 46-47
Comment:
**`vitest` version pinned to `^4.0.13` — verify this is intentional**
Vitest `^4.x` is a very recent major release. Confirm this version is available and stable in your environment; the `^` range will pick up any `4.x` patch/minor automatically, which may be fine, but it is worth validating the major version bump from the more widely-used `^2.x` series is deliberate and that the API used in `vitest.config.ts` (`configDefaults`, `defineConfig`) is still compatible.
How can I resolve this? If you propose a fix, please make it concise.…e --raw, enforce --json/--table exclusion - Use parseAsync() instead of parse() so async command handlers are properly awaited - Import version from package.json instead of hardcoding '0.1.0' - Remove --raw flag (duplicated --format raw) - Add mutual exclusion check for --json and --table in preAction hook
|
Target branch is in the excluded branches list. |
There was a problem hiding this comment.
💡 Codex Review
API/sdks/typescript-sdk/src/client.ts
Line 641 in 2d278e4
The transport-event values in docs/openapi.json use names like container.transport.rail_unloaded, but this filter only matches an rail. prefix. As written, getRailMilestones() drops every rail event, so t49 containers rail will always report rail_events: [] even when the container has rail milestones in the API response.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const require = createRequire(import.meta.url); | ||
| const { version } = require('../package.json') as { version: string }; |
There was a problem hiding this comment.
Load the CLI version from the package root
After tsc, this module is emitted as dist/src/index.js, so require('../package.json') resolves to dist/package.json. We do not emit that file anywhere, which means every built invocation of the CLI (node dist/bin/t49.js, a packed tarball, or a published package) will fail with MODULE_NOT_FOUND before any command runs.
Useful? React with 👍 / 👎.
| data: { | ||
| type: 'custom_field', | ||
| id: fieldId, | ||
| value, |
There was a problem hiding this comment.
Build custom-field payloads with
attributes.api_slug
docs/openapi.json defines both POST /shipments/{shipment_id}/custom_fields and POST /containers/{container_id}/custom_fields with a body shaped like data.attributes.{api_slug,value}. This code sends data.id and data.value instead, so shipments set-custom-field and the matching container command will submit a schema-invalid payload and get a 4xx response on every write. The same malformed payload is duplicated in setContainerCustomField.
Useful? React with 👍 / 👎.
- Add missing .js extension to config import in client-factory.ts - Fix CliConfig → Record<string, unknown> cast in writeConfig - Fix reduce() generic type in fields.ts getValue() - Fix workspace:* → * for npm workspace compatibility
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Comprehensive plan for building a CLI on top of @terminal49/sdk targeting
LLM agents, chat interfaces, and human developers. Includes:
design patterns, error handling, and 4-phase implementation roadmap
targets enforced via vitest thresholds
directory structure with annotated placeholder modules
https://claude.ai/code/session_01361NQzDnx8FQSPizmZmjrW
Greptile Summary
This PR scaffolds a comprehensive CLI tool (
@terminal49/cli) built on top of the existing TypeScript SDK, designed for dual consumption by LLM agents and human developers.What Changed:
--fields)Notable Design Decisions:
{ok, command, data, pagination, meta}) for LLM consumption--compactand--fieldsflags for agent workflowsIssues Identified in Previous Threads:
bin/t49.ts:12usesprogram.parse()instead ofparseAsync()— async command handlers won't be awaitedsrc/index.ts:36hardcodes version0.1.0instead of importing frompackage.jsonsrc/index.ts:40,46has duplicate flags:--rawand--format rawrepresent the same output modesrc/index.ts:38,39has conflicting--jsonand--tableflags without mutual exclusion enforcementConfidence Score: 4/5
bin/t49.tsandsrc/index.tswhich contain the issues identified in previous review threadsImportant Files Changed
program.parse()instead ofparseAsync()(async command handlers won't be awaited)--rawduplicates--format raw,--json/--tablenot mutually exclusive)Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Start([User runs t49 command]) --> Parse[bin/t49.ts: createProgram & parse args] Parse --> Hook[preAction hook: set auth scheme env var] Hook --> Factory[client-factory: resolve token<br/>flag > env > config file] Factory --> Config{Token found?} Config -->|No| AuthError[Throw AuthenticationError<br/>exit code 3] Config -->|Yes| CreateClient[Create Terminal49Client<br/>with token, baseUrl, format] CreateClient --> Command[Execute command action<br/>e.g., containers.get] Command --> SDK[Call @terminal49/sdk method] SDK --> Error{Error?} Error -->|Yes| MapError[errors.ts: map SDK error<br/>to CLI exit code] MapError --> FormatError[formatter: output JSON/table error] FormatError --> Exit1[process.exit with mapped code] Error -->|No| FormatSuccess[formatter: detect TTY<br/>apply --fields projection] FormatSuccess --> Output{Output mode?} Output -->|--json or pipe| JSON[json.ts: success envelope] Output -->|--table or TTY| Table[table.ts: render table] JSON --> Stdout[Write to stdout] Table --> Stdout Stdout --> Exit0[exit 0]Last reviewed commit: 45a9f42